fix(onboard): retry terminated forward listeners - #7267
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe forward-start flow recognizes definitive listener-start failures, checks sandbox ownership and port liveness, and retries eligible failures with bounded attempts. Agent and dashboard onboarding use the new retry helper, with sandbox-scoped cleanup and expanded tests. ChangesForward startup retry handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Onboarding
participant ForwardStart
participant OpenShell
participant PortProbe
Onboarding->>ForwardStart: start detached forward with retries
ForwardStart->>OpenShell: start SSH forward and fetch forward list
OpenShell-->>ForwardStart: listener diagnostic and ownership rows
ForwardStart->>PortProbe: check local port after listener failure
PortProbe-->>ForwardStart: live or closed
ForwardStart-->>Onboarding: success or bounded retry outcome
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 0f6fba2 in the TypeScript / code-coverage/cliThe overall coverage in commit 0f6fba2 in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard/forward-start.ts (1)
326-346: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerified against tests — logic is correct.
Traced the
listedOwner/listedForAnotherSandboxcomputation and the port-conflict → listener-start-failure → untracked-forward branch ordering against every new test case (foreign ownership rejection, ControlMaster live-port exception, bounded retries). All outcomes match. One minor readability note: this block plus the retry predicate inrunDetachedForwardStartWithRetries(Lines 393-424) are both flagged as high-complexity by the diff tooling, and the coding guidelines ask to keep function complexity low. Consider extracting the classification (spawn-conflict/listener-start-failure/ok-port-livedecision) into a small named helper to keeprunDetachedForwardStartWithDiagnostics's loop body flatter, though the current early-return control flow makes this non-trivial to extract cleanly.As per coding guidelines, "Keep function complexity low and avoid introducing unnecessary complexity hotspots."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/onboard/forward-start.ts` around lines 326 - 346, Reduce complexity in runDetachedForwardStartWithDiagnostics by extracting the listedOwner/port-conflict/listener-start-failure classification into a small named helper that preserves the existing spawn-conflict, listener-start-failure, ok-port-live, and normal-success outcomes. Keep retry behavior in runDetachedForwardStartWithRetries unchanged and retain the current early-return semantics.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lib/onboard/forward-start.ts`:
- Around line 326-346: Reduce complexity in
runDetachedForwardStartWithDiagnostics by extracting the
listedOwner/port-conflict/listener-start-failure classification into a small
named helper that preserves the existing spawn-conflict, listener-start-failure,
ok-port-live, and normal-success outcomes. Keep retry behavior in
runDetachedForwardStartWithRetries unchanged and retain the current early-return
semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5256a50c-8531-44c3-adf6-7df259831eba
📒 Files selected for processing (4)
src/lib/onboard/agent-fixed-forward.tssrc/lib/onboard/dashboard.tssrc/lib/onboard/forward-start.test.tssrc/lib/onboard/forward-start.ts
wscurran
added
area: networking
dfernandez365-rgb
left a comment
dfernandez365-rgb
left a comment
There was a problem hiding this comment.
Security review — changes required before merge.
The new success and retry paths do not prove listener ownership:
- A port that is closed before spawn and TCP-live afterward is temporal correlation, not ownership. A foreign process can bind in that interval and be accepted as ok-port-live.
- fetchForwardList() errors are collapsed to an empty list. A gateway authentication/authorization/access failure can therefore combine with listener diagnostics and a live foreign port to return success.
- Retry cleanup waits and then stops by current sandboxName + port, not immutable attempt identity. It can stop a newer concurrent forward after OpenShell has already terminated the failed child.
Please require structured attempt/listener identity or an authenticated application-level challenge, fail closed whenever ownership enumeration fails, and avoid cleanup unless it is bound to an immutable attempt ID/PID/process identity. Add tests for access-denied plus live port, a foreign bind in the pre/post window, concurrent same-target replacement, and the positive-PID termination path. The current false-to-true mock encodes the race as proof rather than distinguishing it.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Adds the canonical pre-tag `## v0.0.95` release entry to `docs/changelog/2026-07-24.mdx`, before the existing v0.0.94 entry. The entry summarizes approved user-visible changes merged since v0.0.94 and excludes internal-only prerequisites. ## Changes - Adds the v0.0.95 summary and detailed bullets for gateway lifecycle, recovery, state transfer, inference compatibility, sandbox security, Discord policy, and E2E evidence. - Links each user-facing theme to the most specific published documentation. - Records the release entry in the shared native changelog used by the OpenClaw, Hermes, and Deep Agents guides. Source summary: - [#7246](#7246), [#7228](#7228), [#7267](#7267), [#7489](#7489), [#7509](#7509), [#7351](#7351), and [#7290](#7290) -> `docs/changelog/2026-07-24.mdx`: Gateway authority, forward teardown and retry, managed recovery, Hermes restart recovery, scoped uninstall, and orphan-aware backup behavior. - [#7344](#7344) and [#7416](#7416) -> `docs/changelog/2026-07-24.mdx`: Atomic SQLite restore and host download verification. - [#7476](#7476), [#7347](#7347), [#7281](#7281), [#7485](#7485), [#7491](#7491), and [#7422](#7422) -> `docs/changelog/2026-07-24.mdx`: Windows Ollama reuse, CDI fallback, bounded OpenRouter connection setup, Nemotron-3 request compatibility, and managed Deep Agents retry and provider-error behavior. - [#6884](#6884), [#7481](#7481), [#6878](#6878), [#7467](#7467), [#7502](#7502), [#7503](#7503), [#7504](#7504), and [#7486](#7486) -> `docs/changelog/2026-07-24.mdx`: Trusted base-image overrides, local rebuild images, runtime validation, config preservation, reviewed package updates, and fewer final-image payload layers. - [#7303](#7303) -> `docs/changelog/2026-07-24.mdx`: Scoped Discord application-command management. - [#7488](#7488), [#7465](#7465), [#7497](#7497), [#7464](#7464), [#7501](#7501), [#7494](#7494), and [#7493](#7493) -> `docs/changelog/2026-07-24.mdx`: Selected-test risk signals, retry cleanup, full root-image validation, direct-main Hermes setup, executed PR-gate evidence, nightly history, and runner wait reporting. - [#7447](#7447) is an internal pinned-runtime prerequisite and is intentionally excluded from canonical supported-integration documentation. - [#7370](#7370) adds maintainer-only advisory reconciliation tooling and does not change supported user behavior. - [#7495](#7495) updates existing documentation and does not add a new v0.0.95 behavior claim. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [x] Existing tests cover changed behavior — justification: `test/changelog-docs.test.ts` validates the dated changelog structure, heading uniqueness, and published links. - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: `docs/changelog/2026-07-24.mdx`; writing rules, documentation style, factual release meaning, and published links reviewed at exact head `58b02f2bf`. - Agent: Codex documentation writer reviewer <!-- docs-review-head-sha: 58b02f2 --> <!-- docs-review-agents-blob-sha: 9c9b36d --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: `npx vitest run test/changelog-docs.test.ts` passed 6 tests. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — the build passed with 0 errors and 2 Fern warnings. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added a new v0.0.95 changelog entry above v0.0.94. * Documented improved externally supervised gateway lifecycle ownership. * Improved snapshot restore reliability and SQLite state handling. * Tightened CLI `backup-all` behavior and host artifact verification. * Updated Windows onboarding guidance (including Ollama service reuse and CDI directory fallback). * Noted inference compatibility fixes, deeper agent failure classification, stricter base-image validation, updated Discord bot command permissions, and refined E2E release automation evidence handling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
6 participants
Summary
OpenShell can terminate a dashboard-forward attempt after its local listener fails to open, while NemoClaw continues polling the dead attempt for three minutes. This change recognizes that definitive failure, verifies the port is still closed, and performs bounded sandbox-scoped cleanup and retries while preserving the existing live ControlMaster exception.
Related Issue
Fixes #7266
Changes
ssh exited before local forward listener openedandlocal forward listener did not opendiagnostics instead of polling an attempt that cannot recover.Type of Change
Quality Gates
DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailableforward-start.test.ts: 35 passed; dashboard/messaging caller suite: 86 passed; dashboard integration suite: 46 passed;npm run typecheck:clipassed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Charan Jagwani cjagwani@nvidia.com
Summary by CodeRabbit